Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Disable new tab background for tests #9489

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Disable new tab background for tests #9489

merged 1 commit into from
Jun 21, 2017

Conversation

cezaraugusto
Copy link
Contributor

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Auditors: @bsclifton
Fix: #9240

Most tests were created while new tab was a blank page and/or before we had the option to show gradient background.

Test Plan:

  1. While running tests, new tab background should be shown as background gradient rather than the background image.
  2. No test regressions should happen

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@cezaraugusto cezaraugusto added this to the 0.19.x (Nightly Channel) milestone Jun 15, 2017
@cezaraugusto cezaraugusto self-assigned this Jun 15, 2017
NejcZdovc
NejcZdovc suggested changes Jun 15, 2017
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go other way around. So I think that non of the tests need background so I would disable it on global level, so if NODE_ENV test set this value to false or when you initialize test in lib/brave. This way tests are lighter and you can enable if needed.

luixxiul
luixxiul reviewed Jun 16, 2017
View reviewed changes
Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't documentation needed on docs/ to let people know that the images are disabled by default and they need to set something to run tests that include dashboard images?

luixxiul
luixxiul reviewed Jun 19, 2017
View reviewed changes
Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ on docs update :-)

@NejcZdovc
Copy link
Contributor

@cezaraugusto can you please squash commits

bsclifton
bsclifton approved these changes Jun 21, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change; this will help save testing time! 😄

@bsclifton
Copy link
Member

@NejcZdovc can you please re-review (and merge if it looks OK 😄 )

NejcZdovc
NejcZdovc approved these changes Jun 21, 2017
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ++

@NejcZdovc NejcZdovc merged commit 63ff18d into master Jun 21, 2017
@cezaraugusto cezaraugusto deleted the tests/9240 branch June 21, 2017 08:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@luixxiul luixxiul luixxiul left review comments

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@bsclifton bsclifton bsclifton approved these changes

Assignees

@cezaraugusto cezaraugusto

Labels
automated-tests
Projects
None yet
Milestone
0.19.x (Beta Channel)
Development

Successfully merging this pull request may close these issues.
4 participants
@cezaraugusto @NejcZdovc @bsclifton @luixxiul